-
Notifications
You must be signed in to change notification settings - Fork 133
[Woo POS][Local Catalog] Catalog full sync required #14678
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Woo POS][Local Catalog] Catalog full sync required #14678
Conversation
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #14678 +/- ##
============================================
- Coverage 38.05% 38.04% -0.01%
- Complexity 9997 10020 +23
============================================
Files 2120 2124 +4
Lines 119445 119716 +271
Branches 16254 16302 +48
============================================
+ Hits 45449 45545 +96
- Misses 69411 69574 +163
- Partials 4585 4597 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…n-pos-splash' into woomob-1412-woo-poslocal-catalog-full-sync-overdue-handling
Generated by 🚫 Danger |
That was fixed 👍
Let me improve 3. and 4. in a separate PR, as this one grew too big—sorry for that, BTW! I created task for this: WOOMOB-1525
Good catch. This was fixed 👍 Thank you for the review, @malinajirka! I addressed your comments. Also, sorry for the long PR I failed to keep it within a limit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...rc/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFullSyncStatusChecker.kt
Show resolved
Hide resolved
...kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosPerformInstantCatalogFullSync.kt
Outdated
Show resolved
Hide resolved
...kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosPerformInstantCatalogFullSync.kt
Outdated
Show resolved
Hide resolved
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/splash/WooPosSplashScreen.kt
Show resolved
Hide resolved
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/WooPosItemsScreen.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
|
Version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @samiuelson for iterating on the PR! I've reviewed the code and it all looks good to me. However, I've encountered a couple issues when testing the PR. I'd consider merging it as is and fixing the bugs in a separate PR, wdyt?
Infinite splash screen
- Downgrade your site to WooCommerce 10.2 or older
- Clean apps data
- Login
- Enter POS
- Notice catalog is syncing -> notice
/variationsrequests fail since the endpoint isn't available in v10.2 => the logs mentionWorker result RETRYwhich isn't properly handled (it goes again intoENQUEUEDstate on Retry).
POS Opened with empty catalog
- Clean apps data
- Login
- Turn off wifi/data
- Open POS
- Notice expected error
- Tap on retry
- Turn on wifi/data
- Tap on retry
- Notice you are allowed to enter POS before the initial sync completes.
My.Movie.mp4
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/splash/WooPosSplashViewModel.kt
Outdated
Show resolved
Hide resolved
| abstract suspend fun getProduct(localSiteId: LocalId, remoteId: RemoteId): WooPosProductEntity? | ||
|
|
||
| @Query("SELECT COUNT(*) FROM PosProductEntity WHERE localSiteId = :localSiteId") | ||
| abstract suspend fun getProductCount(localSiteId: LocalId): Int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📓 I think this is fine for the current use case, but we need to be careful, since getProductCount returns count of all products including those which are not supported by the POS (hidden in the POS).
| wooPosLogWrapper.e("Full sync check failed: No site selected") | ||
| return WooPosFullSyncRequirement.Error("No site selected") | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 I'm wondering if we need to check isPeriodicSyncEnabledForSite here as well. Otherwise, this class might require sync while the worker always finishes with failure on line WooPosLocalCatalogSyncWorker::54. Wdyt?
P.S. Feel free to create a separate ticket.
Task created: WOOMOB-1536
Task created: WOOMOB-1537 I appreciate the thorough review, Jirka! I added follow-up tasks in linear: WOOMOB-1538, WOOMOB-1537, WOOMOB-1536. |
…due-warning [Woo POS][Local Catalog] Full sync overdue warning banner
WOOMOB-1412
Description
The goal of PR is to improve the reliability of the full sync of POS local catalog.
The full sync is scheduled to be performed every 24h, at night. However, in case the device was turned off at night, the sync may not succeed. To handle such case, this PR adds:
trunk.Steps to reproduce
The full sync should be performed in a blocking way before allowing access to the POS for the first time (or in case product data is missing)
Testing information
The tests that have been performed
Above.
Images/gif
Screen_recording_20251007_181723.mp4
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.